Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ROS 2 node that flips image and publishes updated transform #756

Closed
wants to merge 16 commits into from

Conversation

dcconner
Copy link

Updating for latest changes to Rolling release (Humble compatible) and closing PR #743 from older branch

(e.g. for camera mounted upside down)

The node does a simple image rotation (by 90, 180, 270) as simple pixel transpose .

The node publishes a static tf to allow images to be viewed right side up in RViz if camera is mounted upside down.

This is less general than image_rotate, but performs simpler operations.

It also preserves the image size and aspect ratio.

It does NOT attempt to remap the distortion parameters at this time.

The code has been tested under Humble. Still required camera topics as parameters due to issue with extracting camera/camera_info names from base topic.

Posting pull request for review and comment. It does not currently have any specific unit tests.

Copy link
Collaborator

@JWhitleyWork JWhitleyWork left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few comments.

image_flip/cfg/ImageFlip.cfg Outdated Show resolved Hide resolved
image_flip/CHANGELOG.rst Outdated Show resolved Hide resolved
image_flip/launch/image_flip.launch.py Outdated Show resolved Hide resolved
@dcconner
Copy link
Author

dcconner commented Dec 8, 2022

@JWhitleyWork I think I have cleaned up most, but the header out of order has me flummoxed. I thought I have matched image rotate. I think I have abused Jenkins enough for tonight. Let me know if you want further cleanup, and if you'd prefer me to re-push as a single squashed commit for this PR.

#include <math.h>

#include <memory>
#include <string>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Move these two below line 43 to resolve the linter error.

Copy link
Collaborator

@JWhitleyWork JWhitleyWork left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, just a couple more minor things. After this it should pass CI and I'll be happy to merge it.

config_.in_image_topic_name =
this->declare_parameter("in_image_topic_name", std::string("image"));
config_.out_image_topic_name =
this->declare_parameter("out_image_topic_name", std::string("rotated_image"));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For topics which don't use a configurable namespace, please allow users to use remapping instead of passing in the topic name as a parameter. This causes confusion and may make users frustrated if they try to use remapping but the name has been changed with configuration parameters.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIRC, using a parameter was a hack around an issue with the rclcpp resolve_topic_name and how it dealt with images. It will be a bit before I can dig into that again. Do you happen to know if the resolve_topic_name issue was resolved?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's fixed now. During testing I found I could not use the image_view, and rviz2 was crashing with rolling, so I added a parameter to let you select QoS settings for input and output topics. Defaults to sensor_data (best effort) on input, and default (reliable) on output.

@dcconner
Copy link
Author

rebased to latest rolling branch

@mikeferguson mikeferguson self-assigned this Feb 11, 2024
@mikeferguson
Copy link
Member

Assigning myself to review this and get it merged (I'm inclined though to not add yet another package - maybe make this another node in image_rotate? just because of the overhead of each package with separate build/documentation/etc)

@dcconner
Copy link
Author

@mikeferguson It will probably be May before I would have to work on reorganizing this into "another node in image_rotate"

@mikeferguson mikeferguson mentioned this pull request Feb 12, 2024
5 tasks
@mikeferguson
Copy link
Member

@DCConnor - not a problem, I've opened #942 where I continued from your branch

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants